diff mbox series

[net-next,v3,6/8] net: pcs: Add 10GBASE-R mode for Synopsys Designware XPCS

Message ID 20230419082739.295180-7-jiawenwu@trustnetic.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series TXGBE PHYLINK support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 45 this patch: 45
netdev/cc_maintainers warning 6 maintainers not CCed: pabeni@redhat.com hkallweit1@gmail.com kuba@kernel.org edumazet@google.com andrew@lunn.ch davem@davemloft.net
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 45 this patch: 45
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 119 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiawen Wu April 19, 2023, 8:27 a.m. UTC
Add basic support for XPCS using 10GBASE-R interface. This mode will
be extended to use interrupt, so set pcs.poll false. And avoid soft
reset so that the device using this mode is in the default configuration.

Cc: Jose Abreu <Jose.Abreu@synopsys.com>

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/pcs/pcs-xpcs.c   | 58 ++++++++++++++++++++++++++++++++++++
 include/linux/pcs/pcs-xpcs.h |  1 +
 2 files changed, 59 insertions(+)

Comments

Vladimir Oltean April 19, 2023, 1:19 p.m. UTC | #1
On Wed, Apr 19, 2023 at 04:27:37PM +0800, Jiawen Wu wrote:
> Add basic support for XPCS using 10GBASE-R interface. This mode will
> be extended to use interrupt, so set pcs.poll false. And avoid soft
> reset so that the device using this mode is in the default configuration.

I'm not clear why the xpcs_soft_reset() call is avoided. Isn't the
out-of-reset configuration the "default" one?

> +static int xpcs_get_state_10gbaser(struct dw_xpcs *xpcs,
> +				   struct phylink_link_state *state)
> +{
> +	int ret;
> +
> +	state->link = false;
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT1);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & MDIO_STAT1_LSTATUS)
> +		state->link = true;
> +
> +	if (state->link) {

It seems pointless to open a new "if" statement when this would have
sufficed:

	if (ret & MDIO_STAT1_LSTATUS) {
		state->link = true;
		state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX;
		...
	}

> +		state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX;
> +		state->duplex = DUPLEX_FULL;
> +		state->speed = SPEED_10000;
> +	}
> +
> +	return 0;
> +}
Jiawen Wu April 20, 2023, 1:56 a.m. UTC | #2
On Wednesday, April 19, 2023 9:20 PM, Vladimir Oltean wrote:
> On Wed, Apr 19, 2023 at 04:27:37PM +0800, Jiawen Wu wrote:
> > Add basic support for XPCS using 10GBASE-R interface. This mode will
> > be extended to use interrupt, so set pcs.poll false. And avoid soft
> > reset so that the device using this mode is in the default configuration.
> 
> I'm not clear why the xpcs_soft_reset() call is avoided. Isn't the
> out-of-reset configuration the "default" one?
> 

Theoretically so, I need to configure 10GBASE-R mode after reset. But this
configuration involves board info to configure PMA, etc., I'd like to implement
it in the next patch. Now the "default" configuration refers to the mode in
which the firmware is configured.

> > +static int xpcs_get_state_10gbaser(struct dw_xpcs *xpcs,
> > +				   struct phylink_link_state *state)
> > +{
> > +	int ret;
> > +
> > +	state->link = false;
> > +
> > +	ret = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (ret & MDIO_STAT1_LSTATUS)
> > +		state->link = true;
> > +
> > +	if (state->link) {
> 
> It seems pointless to open a new "if" statement when this would have
> sufficed:
> 
> 	if (ret & MDIO_STAT1_LSTATUS) {
> 		state->link = true;
> 		state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX;
> 		...
> 	}
> 
> > +		state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX;
> > +		state->duplex = DUPLEX_FULL;
> > +		state->speed = SPEED_10000;
> > +	}
> > +
> > +	return 0;
> > +}
>
Vladimir Oltean April 20, 2023, 8:03 a.m. UTC | #3
On Thu, Apr 20, 2023 at 09:56:26AM +0800, Jiawen Wu wrote:
> On Wednesday, April 19, 2023 9:20 PM, Vladimir Oltean wrote:
> > On Wed, Apr 19, 2023 at 04:27:37PM +0800, Jiawen Wu wrote:
> > > Add basic support for XPCS using 10GBASE-R interface. This mode will
> > > be extended to use interrupt, so set pcs.poll false. And avoid soft
> > > reset so that the device using this mode is in the default configuration.
> > 
> > I'm not clear why the xpcs_soft_reset() call is avoided. Isn't the
> > out-of-reset configuration the "default" one?
> 
> Theoretically so, I need to configure 10GBASE-R mode after reset. But this
> configuration involves board info to configure PMA, etc., I'd like to implement
> it in the next patch. Now the "default" configuration refers to the mode in
> which the firmware is configured.

How much extra complexity are we talking about, to not depend on the
configuration done by the bootloader?
Jiawen Wu April 20, 2023, 8:38 a.m. UTC | #4
On Thursday, April 20, 2023 4:03 PM, Vladimir Oltean wrote:
> On Thu, Apr 20, 2023 at 09:56:26AM +0800, Jiawen Wu wrote:
> > On Wednesday, April 19, 2023 9:20 PM, Vladimir Oltean wrote:
> > > On Wed, Apr 19, 2023 at 04:27:37PM +0800, Jiawen Wu wrote:
> > > > Add basic support for XPCS using 10GBASE-R interface. This mode will
> > > > be extended to use interrupt, so set pcs.poll false. And avoid soft
> > > > reset so that the device using this mode is in the default configuration.
> > >
> > > I'm not clear why the xpcs_soft_reset() call is avoided. Isn't the
> > > out-of-reset configuration the "default" one?
> >
> > Theoretically so, I need to configure 10GBASE-R mode after reset. But this
> > configuration involves board info to configure PMA, etc., I'd like to implement
> > it in the next patch. Now the "default" configuration refers to the mode in
> > which the firmware is configured.
> 
> How much extra complexity are we talking about, to not depend on the
> configuration done by the bootloader?
> 

It needs to implement compat->pma_config, and add a flag in struct dw_xpcs
to indicate board with specific pma configuration. For 10GBASE-R interface, it
relatively simple, but a bit more complicate for 1000BASE-X since there are
logic conflicts in xpcs_do_config(), I haven't resolved yet.

In addition, reconfiguring XPCS will cause some known issues that I need to
workaround in the ethernet driver. So I'd like to add configuration when I
implement rate switching.

There is a piece codes for my test:

+static int xpcs_read_pma(struct dw_xpcs *xpcs, int reg)
+{
+	return xpcs_read(xpcs, MDIO_MMD_PMAPMD, DW_PMA_MMD + reg);
+}
+
+static int xpcs_write_pma(struct dw_xpcs *xpcs, int reg, u16 val)
+{
+	return xpcs_write(xpcs, MDIO_MMD_PMAPMD, DW_PMA_MMD + reg, val);
+}
+
+static int xpcs_poll_power_up(struct dw_xpcs *xpcs)
+{
+	int val, ret;
+
+	/* Wait xpcs power-up good */
+	ret = read_poll_timeout(xpcs_read_vpcs, val,
+				(val & DW_VR_XS_PCS_DIG_STS_PSEQ_ST) ==
+				DW_VR_XS_PCS_DIG_STS_PSEQ_ST_GOOD,
+				10000, 1000000, false,
+				xpcs, DW_VR_XS_PCS_DIG_STS);
+	if (ret < 0)
+		pr_err("%s: xpcs power-up timeout\n", __func__);
+
+	return ret;
+}
+
+static int xpcs_pma_init_done(struct dw_xpcs *xpcs)
+{
+	int val, ret;
+
+	xpcs_write_vpcs(xpcs, DW_VR_XS_PCS_DIG_CTRL1,
+			DW_VR_XS_PCS_DIG_CTRL1_VR_RST |
+			DW_VR_XS_PCS_DIG_CTRL1_EN_VSMMD1);
+
+	/* wait pma initialization done */
+	ret = read_poll_timeout(xpcs_read_vpcs, val,
+				!(val & DW_VR_XS_PCS_DIG_CTRL1_VR_RST),
+				100000, 10000000, false,
+				xpcs, DW_VR_XS_PCS_DIG_CTRL1);
+	if (ret < 0)
+		pr_err("%s: xpcs pma initialization timeout\n", __func__);
+
+	return ret;
+}
+
+static int xpcs_10gbaser_pma_config_wx(struct dw_xpcs *xpcs)
+{
+	int val, ret;
+
+	ret = xpcs_poll_power_up(xpcs);
+	if (ret < 0)
+		return ret;
+
+	xpcs_write(xpcs, MDIO_MMD_PCS, MDIO_CTRL2, MDIO_PCS_CTRL2_10GBR);
+	val = xpcs_read(xpcs, MDIO_MMD_PMAPMD, MDIO_CTRL1);
+	val |= MDIO_CTRL1_SPEED10G;
+	xpcs_write(xpcs, MDIO_MMD_PMAPMD, MDIO_CTRL1, val);
+
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_MPLLA_CTL0, 0x21);
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_MPLLA_CTL3, 0);
+	val = xpcs_read_pma(xpcs, DW_VR_XS_PMA_TX_GENCTL1);
+	val = u16_replace_bits(val, 0x5, DW_VR_XS_PMA_TX_GENCTL1_VBOOST_LVL);
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_TX_GENCTL1, val);
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_MISC_CTL0, 0xCF00);
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_VCO_CAL_LD0, 0x549);
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_VCO_CAL_REF0, 0x29);
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_TX_RATE_CTL, 0);
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_RX_RATE_CTL, 0);
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_TX_GEN_CTL2, 0x300);
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_RX_GEN_CTL2, 0x300);
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_MPLLA_CTL2, 0x600);
+
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_RX_EQ_CTL0, 0x45);
+	val = xpcs_read_pma(xpcs, DW_VR_XS_PMA_RX_EQ_ATTN_CTL);
+	val &= ~DW_VR_XS_PMA_RX_EQ_ATTN_LVL0;
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_RX_EQ_ATTN_CTL, val);
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_DFE_TAP_CTL0, 0xBE);
+	val = xpcs_read_pma(xpcs, DW_VR_XS_PMA_AFE_DFE_ENABLE);
+	val &= ~(DW_VR_XS_PMA_DFE_EN_0 | DW_VR_XS_PMA_AFE_EN_0);
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_AFE_DFE_ENABLE, val);
+	val = xpcs_read_pma(xpcs, DW_VR_XS_PMA_RX_EQ_CTL4);
+	val &= ~DW_VR_XS_PMA_RX_EQ_CTL4_CONT_ADAPT0;
+	xpcs_write_pma(xpcs, DW_VR_XS_PMA_RX_EQ_CTL4, val);
+
+	return xpcs_pma_init_done(xpcs);
+}
+
+static int xpcs_10gbaser_pma_config(struct dw_xpcs *xpcs)
+{
+	if (xpcs->flags & DW_MODEL_WANGXUN_SP)
+		return xpcs_10gbaser_pma_config_wx(xpcs);
+
+	return 0;
+}
+
Vladimir Oltean April 20, 2023, 8:52 a.m. UTC | #5
On Thu, Apr 20, 2023 at 04:38:48PM +0800, Jiawen Wu wrote:
> It needs to implement compat->pma_config, and add a flag in struct dw_xpcs
> to indicate board with specific pma configuration. For 10GBASE-R interface, it
> relatively simple, but a bit more complicate for 1000BASE-X since there are
> logic conflicts in xpcs_do_config(), I haven't resolved yet.
> 
> In addition, reconfiguring XPCS will cause some known issues that I need to
> workaround in the ethernet driver. So I'd like to add configuration when I
> implement rate switching.
> 
> There is a piece codes for my test:

The PMA initialization procedure looks pretty clean to me (although I'm
not clear why it depends upon xpcs->flags & DW_MODEL_WANGXUN_SP when the
registers seem to be present in the common databook), and having it in
the XPCS driver seems much preferable to depending on an unknown previous
initialization stage.

Could you detail a bit the known issues and the 1000BASE-X conflicts in
xpcs_do_config()?
Jiawen Wu April 20, 2023, 9:32 a.m. UTC | #6
On Thursday, April 20, 2023 4:52 PM, Vladimir Oltean wrote:
> On Thu, Apr 20, 2023 at 04:38:48PM +0800, Jiawen Wu wrote:
> > It needs to implement compat->pma_config, and add a flag in struct dw_xpcs
> > to indicate board with specific pma configuration. For 10GBASE-R interface, it
> > relatively simple, but a bit more complicate for 1000BASE-X since there are
> > logic conflicts in xpcs_do_config(), I haven't resolved yet.
> >
> > In addition, reconfiguring XPCS will cause some known issues that I need to
> > workaround in the ethernet driver. So I'd like to add configuration when I
> > implement rate switching.
> >
> > There is a piece codes for my test:
> 
> The PMA initialization procedure looks pretty clean to me (although I'm
> not clear why it depends upon xpcs->flags & DW_MODEL_WANGXUN_SP when the
> registers seem to be present in the common databook), and having it in
> the XPCS driver seems much preferable to depending on an unknown previous
> initialization stage.

The values configured in PMA depend on the board signal quality, Synopsys once
provided the values based on our board information, but we don't know the details
of the computation. So I don't think it's universal.

> 
> Could you detail a bit the known issues and the 1000BASE-X conflicts in
> xpcs_do_config()?
> 

Known issue is that traffic must be totally stopped while the PMA is being configured.
And XPCS should add a judgment that PMA only need to be reconfigured when
interface is changed.

In 1000BASE-X interface, for my current testing, PMA configuration should precede
AN configuration, and need to set PCS_DIG_CTRL1 reg? My test code for AN config:

+static int xpcs_config_aneg_c37_1000basex_wx(struct dw_xpcs *xpcs, unsigned int mode,
+					     const unsigned long *advertising)
+{
+
+	xpcs_write(xpcs, MDIO_MMD_PCS, DW_VR_MII_DIG_CTRL1, 0x3002);
+	xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL, 0x0109);
+	xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, 0x0200);
+	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1);
+	ret |= BMCR_ANENABLE;
+	xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, ret);
+}
diff mbox series

Patch

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 539cd43eae8d..58a417edfb4d 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -64,6 +64,16 @@  static const int xpcs_xlgmii_features[] = {
 	__ETHTOOL_LINK_MODE_MASK_NBITS,
 };
 
+static const int xpcs_10gbaser_features[] = {
+	ETHTOOL_LINK_MODE_Pause_BIT,
+	ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+	ETHTOOL_LINK_MODE_10000baseSR_Full_BIT,
+	ETHTOOL_LINK_MODE_10000baseLR_Full_BIT,
+	ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT,
+	ETHTOOL_LINK_MODE_10000baseER_Full_BIT,
+	__ETHTOOL_LINK_MODE_MASK_NBITS,
+};
+
 static const int xpcs_sgmii_features[] = {
 	ETHTOOL_LINK_MODE_Pause_BIT,
 	ETHTOOL_LINK_MODE_Asym_Pause_BIT,
@@ -106,6 +116,10 @@  static const phy_interface_t xpcs_xlgmii_interfaces[] = {
 	PHY_INTERFACE_MODE_XLGMII,
 };
 
+static const phy_interface_t xpcs_10gbaser_interfaces[] = {
+	PHY_INTERFACE_MODE_10GBASER,
+};
+
 static const phy_interface_t xpcs_sgmii_interfaces[] = {
 	PHY_INTERFACE_MODE_SGMII,
 };
@@ -123,6 +137,7 @@  enum {
 	DW_XPCS_USXGMII,
 	DW_XPCS_10GKR,
 	DW_XPCS_XLGMII,
+	DW_XPCS_10GBASER,
 	DW_XPCS_SGMII,
 	DW_XPCS_1000BASEX,
 	DW_XPCS_2500BASEX,
@@ -246,6 +261,7 @@  static int xpcs_soft_reset(struct dw_xpcs *xpcs,
 
 	switch (compat->an_mode) {
 	case DW_AN_C73:
+	case DW_10GBASER:
 		dev = MDIO_MMD_PCS;
 		break;
 	case DW_AN_C37_SGMII:
@@ -872,6 +888,8 @@  int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
 		return -ENODEV;
 
 	switch (compat->an_mode) {
+	case DW_10GBASER:
+		break;
 	case DW_AN_C73:
 		if (phylink_autoneg_inband(mode)) {
 			ret = xpcs_config_aneg_c73(xpcs, compat);
@@ -919,6 +937,29 @@  static int xpcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	return xpcs_do_config(xpcs, interface, mode, advertising);
 }
 
+static int xpcs_get_state_10gbaser(struct dw_xpcs *xpcs,
+				   struct phylink_link_state *state)
+{
+	int ret;
+
+	state->link = false;
+
+	ret = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT1);
+	if (ret < 0)
+		return ret;
+
+	if (ret & MDIO_STAT1_LSTATUS)
+		state->link = true;
+
+	if (state->link) {
+		state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX;
+		state->duplex = DUPLEX_FULL;
+		state->speed = SPEED_10000;
+	}
+
+	return 0;
+}
+
 static int xpcs_get_state_c73(struct dw_xpcs *xpcs,
 			      struct phylink_link_state *state,
 			      const struct xpcs_compat *compat)
@@ -1033,6 +1074,14 @@  static void xpcs_get_state(struct phylink_pcs *pcs,
 		return;
 
 	switch (compat->an_mode) {
+	case DW_10GBASER:
+		ret = xpcs_get_state_10gbaser(xpcs, state);
+		if (ret) {
+			pr_err("xpcs_get_state_10gbaser returned %pe\n",
+			       ERR_PTR(ret));
+			return;
+		}
+		break;
 	case DW_AN_C73:
 		ret = xpcs_get_state_c73(xpcs, state, compat);
 		if (ret) {
@@ -1188,6 +1237,12 @@  static const struct xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
 		.num_interfaces = ARRAY_SIZE(xpcs_xlgmii_interfaces),
 		.an_mode = DW_AN_C73,
 	},
+	[DW_XPCS_10GBASER] = {
+		.supported = xpcs_10gbaser_features,
+		.interface = xpcs_10gbaser_interfaces,
+		.num_interfaces = ARRAY_SIZE(xpcs_10gbaser_interfaces),
+		.an_mode = DW_10GBASER,
+	},
 	[DW_XPCS_SGMII] = {
 		.supported = xpcs_sgmii_features,
 		.interface = xpcs_sgmii_interfaces,
@@ -1290,6 +1345,9 @@  struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
 		}
 
 		xpcs->pcs.ops = &xpcs_phylink_ops;
+		if (compat->an_mode == DW_10GBASER)
+			return xpcs;
+
 		xpcs->pcs.poll = true;
 
 		ret = xpcs_soft_reset(xpcs, compat);
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index d2da1e0b4a92..61df0c717a0e 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -18,6 +18,7 @@ 
 #define DW_AN_C37_SGMII			2
 #define DW_2500BASEX			3
 #define DW_AN_C37_1000BASEX		4
+#define DW_10GBASER			5
 
 struct xpcs_id;