[PATCH net-next v6 0/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[PATCH net-next v6 0/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[msmulski2]

From: Michal Smulski <michal.smulski@ooma.com>

Changes from previous version:
* use phylink_decode_usxgmii_word() to decode USXGMII link state
* use existing include/uapi/linux/mdio.h defines when parsing status bits

Michal Smulski (1):
  net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

 drivers/net/dsa/mv88e6xxx/chip.c   |  3 +-
 drivers/net/dsa/mv88e6xxx/port.c   |  3 ++
 drivers/net/dsa/mv88e6xxx/serdes.c | 46 ++++++++++++++++++++++++++++--
 drivers/net/dsa/mv88e6xxx/serdes.h |  4 +++
 4 files changed, 52 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[msmulski2]

From: Michal Smulski <michal.smulski@ooma.com>

Enable USXGMII mode for mv88e6393x chips. Tested on Marvell 88E6191X.

Signed-off-by: Michal Smulski <michal.smulski@ooma.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c   |  3 +-
 drivers/net/dsa/mv88e6xxx/port.c   |  3 ++
 drivers/net/dsa/mv88e6xxx/serdes.c | 46 ++++++++++++++++++++++++++++--
 drivers/net/dsa/mv88e6xxx/serdes.h |  4 +++
 4 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 8731af6d79de..5a92ccfd08ea 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -815,8 +815,7 @@ static void mv88e6393x_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
 				config->mac_capabilities |= MAC_5000FD |
 					MAC_10000FD;
 			}
-			/* FIXME: USXGMII is not supported yet */
-			/* __set_bit(PHY_INTERFACE_MODE_USXGMII, supported); */
+			__set_bit(PHY_INTERFACE_MODE_USXGMII, supported);
 		}
 	}
 
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index e9b4a6ea4d09..dd66ec902d4c 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -566,6 +566,9 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 	case PHY_INTERFACE_MODE_10GBASER:
 		cmode = MV88E6393X_PORT_STS_CMODE_10GBASER;
 		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		cmode = MV88E6393X_PORT_STS_CMODE_USXGMII;
+		break;
 	default:
 		cmode = 0;
 	}
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 72faec8f44dc..504424f59795 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -683,7 +683,8 @@ int mv88e6393x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
 	    cmode == MV88E6XXX_PORT_STS_CMODE_SGMII ||
 	    cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX ||
 	    cmode == MV88E6393X_PORT_STS_CMODE_5GBASER ||
-	    cmode == MV88E6393X_PORT_STS_CMODE_10GBASER)
+	    cmode == MV88E6393X_PORT_STS_CMODE_10GBASER ||
+	    cmode == MV88E6393X_PORT_STS_CMODE_USXGMII)
 		lane = port;
 
 	return lane;
@@ -984,7 +985,41 @@ static int mv88e6393x_serdes_pcs_get_state_10g(struct mv88e6xxx_chip *chip,
 			state->speed = SPEED_10000;
 		state->duplex = DUPLEX_FULL;
 	}
+	return 0;
+}
+
+/* USXGMII registers for Marvell switch 88e639x are undocumented and this function is based
+ * on some educated guesses. It appears that there are no status bits related to
+ * autonegotiation complete or flow control.
+ */
+static int mv88e639x_serdes_pcs_get_state_usxgmii(struct mv88e6xxx_chip *chip,
+						  int port, int lane,
+						  struct phylink_link_state *state)
+{
+	u16 status, lp_status;
+	int err;
+
+	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
+				    MV88E6390_USXGMII_PHY_STATUS, &status);
+	if (err) {
+		dev_err(chip->dev, "can't read Serdes USXGMII PHY status: %d\n", err);
+		return err;
+	}
+	dev_dbg(chip->dev, "USXGMII PHY status: 0x%x\n", status);
+
+	state->link = !!(status & MDIO_USXGMII_LINK);
+
+	if (state->link) {
+		err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
+					    MV88E6390_USXGMII_LP_STATUS, &lp_status);
+		if (err) {
+			dev_err(chip->dev, "can't read Serdes USXGMII LP status: %d\n", err);
+			return err;
+		}
+		dev_dbg(chip->dev, "USXGMII LP status: 0x%x\n", lp_status);
 
+		phylink_decode_usxgmii_word(state, lp_status);
+	}
 	return 0;
 }
 
@@ -1020,6 +1055,9 @@ int mv88e6393x_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, int port,
 	case PHY_INTERFACE_MODE_10GBASER:
 		return mv88e6393x_serdes_pcs_get_state_10g(chip, port, lane,
 							   state);
+	case PHY_INTERFACE_MODE_USXGMII:
+		return mv88e639x_serdes_pcs_get_state_usxgmii(chip, port, lane,
+							   state);
 
 	default:
 		return -EOPNOTSUPP;
@@ -1173,6 +1211,7 @@ int mv88e6393x_serdes_irq_enable(struct mv88e6xxx_chip *chip, int port,
 		return mv88e6390_serdes_irq_enable_sgmii(chip, lane, enable);
 	case MV88E6393X_PORT_STS_CMODE_5GBASER:
 	case MV88E6393X_PORT_STS_CMODE_10GBASER:
+	case MV88E6393X_PORT_STS_CMODE_USXGMII:
 		return mv88e6393x_serdes_irq_enable_10g(chip, lane, enable);
 	}
 
@@ -1213,6 +1252,7 @@ irqreturn_t mv88e6393x_serdes_irq_status(struct mv88e6xxx_chip *chip, int port,
 		break;
 	case MV88E6393X_PORT_STS_CMODE_5GBASER:
 	case MV88E6393X_PORT_STS_CMODE_10GBASER:
+	case MV88E6393X_PORT_STS_CMODE_USXGMII:
 		err = mv88e6393x_serdes_irq_status_10g(chip, lane, &status);
 		if (err)
 			return err;
@@ -1477,7 +1517,8 @@ static int mv88e6393x_serdes_erratum_5_2(struct mv88e6xxx_chip *chip, int lane,
 	 * to SERDES operating in 10G mode. These registers only apply to 10G
 	 * operation and have no effect on other speeds.
 	 */
-	if (cmode != MV88E6393X_PORT_STS_CMODE_10GBASER)
+	if (cmode != MV88E6393X_PORT_STS_CMODE_10GBASER &&
+	    cmode != MV88E6393X_PORT_STS_CMODE_USXGMII)
 		return 0;
 
 	for (i = 0; i < ARRAY_SIZE(fixes); ++i) {
@@ -1582,6 +1623,7 @@ int mv88e6393x_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
 		break;
 	case MV88E6393X_PORT_STS_CMODE_5GBASER:
 	case MV88E6393X_PORT_STS_CMODE_10GBASER:
+	case MV88E6393X_PORT_STS_CMODE_USXGMII:
 		err = mv88e6390_serdes_power_10g(chip, lane, on);
 		break;
 	default:
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index 29bb4e91e2f6..e245687ddb1d 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -48,6 +48,10 @@
 #define MV88E6393X_10G_INT_LINK_CHANGE	BIT(2)
 #define MV88E6393X_10G_INT_STATUS	0x9001
 
+/* USXGMII */
+#define MV88E6390_USXGMII_LP_STATUS       0xf0a2
+#define MV88E6390_USXGMII_PHY_STATUS      0xf0a6
+
 /* 1000BASE-X and SGMII */
 #define MV88E6390_SGMII_BMCR		(0x2000 + MII_BMCR)
 #define MV88E6390_SGMII_BMSR		(0x2000 + MII_BMSR)
-- 
2.34.1

Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[Marek Behún]

On Thu,  1 Jun 2023 17:17:04 -0700
msmulski2@gmail.com wrote:

>  				config->mac_capabilities |= MAC_5000FD |
>  					MAC_10000FD;
>  			}
> -			/* FIXME: USXGMII is not supported yet */
> -			/* __set_bit(PHY_INTERFACE_MODE_USXGMII, supported); */
> +			__set_bit(PHY_INTERFACE_MODE_USXGMII, supported);
>  		}
>  	}

The set_bit() should go into the if statement above, since 6361 does not support usxgmii:

 /* 6361 only supports up to 2500BaseX */
 if (!is_6361) {
 	__set_bit(PHY_INTERFACE_MODE_5GBASER, supported);
 	__set_bit(PHY_INTERFACE_MODE_10GBASER, supported);
+	__set_bit(PHY_INTERFACE_MODE_USXGMII, supported);
 	config->mac_capabilities |= MAC_5000FD |
 		MAC_10000FD;
 }
-/* FIXME: USXGMII is not supported yet */
-/* __set_bit(PHY_INTERFACE_MODE_USXGMII, supported); */




Marek

RE: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[Michal Smulski]

Thank you for the review. I will make the change on the next version of this patch.
Michal

-----Original Message-----
From: Marek Behún <kabel@kernel.org> 
Sent: Friday, June 2, 2023 12:54 AM
To: msmulski2@gmail.com
Cc: andrew@lunn.ch; f.fainelli@gmail.com; olteanv@gmail.com; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; linux@armlinux.org.uk; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; simon.horman@corigine.com; Michal Smulski <michal.smulski@ooma.com>
Subject: Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

CAUTION: This email is originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.


On Thu,  1 Jun 2023 17:17:04 -0700
msmulski2@gmail.com wrote:

>                               config->mac_capabilities |= MAC_5000FD |
>                                       MAC_10000FD;
>                       }
> -                     /* FIXME: USXGMII is not supported yet */
> -                     /* __set_bit(PHY_INTERFACE_MODE_USXGMII, supported); */
> +                     __set_bit(PHY_INTERFACE_MODE_USXGMII, 
> + supported);
>               }
>       }

The set_bit() should go into the if statement above, since 6361 does not support usxgmii:

 /* 6361 only supports up to 2500BaseX */  if (!is_6361) {
        __set_bit(PHY_INTERFACE_MODE_5GBASER, supported);
        __set_bit(PHY_INTERFACE_MODE_10GBASER, supported);
+       __set_bit(PHY_INTERFACE_MODE_USXGMII, supported);
        config->mac_capabilities |= MAC_5000FD |
                MAC_10000FD;
 }
-/* FIXME: USXGMII is not supported yet */
-/* __set_bit(PHY_INTERFACE_MODE_USXGMII, supported); */




Marek

Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[Russell King (Oracle)]

On Thu, Jun 01, 2023 at 05:17:04PM -0700, msmulski2@gmail.com wrote:
> +/* USXGMII registers for Marvell switch 88e639x are undocumented and this function is based
> + * on some educated guesses. It appears that there are no status bits related to
> + * autonegotiation complete or flow control.
> + */
> +static int mv88e639x_serdes_pcs_get_state_usxgmii(struct mv88e6xxx_chip *chip,
> +						  int port, int lane,
> +						  struct phylink_link_state *state)
> +{
> +	u16 status, lp_status;
> +	int err;
> +
> +	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> +				    MV88E6390_USXGMII_PHY_STATUS, &status);
> +	if (err) {
> +		dev_err(chip->dev, "can't read Serdes USXGMII PHY status: %d\n", err);
> +		return err;
> +	}
> +	dev_dbg(chip->dev, "USXGMII PHY status: 0x%x\n", status);
> +
> +	state->link = !!(status & MDIO_USXGMII_LINK);
> +
> +	if (state->link) {
> +		err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> +					    MV88E6390_USXGMII_LP_STATUS, &lp_status);

What's the difference between these two registers? Specifically, what
I'm asking is why the USXGMII partner status seems to be split between
two separate registers.

Note that I think phylink_decode_usxgmii_word() is probably missing a
check for MDIO_USXGMII_LINK, based on how Cisco SGMII works (and
USXGMII is pretty similar.)

MDIO_USXGMII_LINK indicates whether the attached PHY has link on the
media side or not. It's entirely possible for the USXGMII link to be
up (thus allowing us to receive the "LPA" from the PHY) but for the
PHY to be reporting to us that it has no media side link.

So, I think phylink_decode_usxgmii_word() at least needs at the
beginning this added:

	if (!(lpa & MDIO_USXGMII_LINK)) {
		state->link = false;
		return;
	}

The only user of this has been the Lynx PCS, so I'll add Ioana to this
email as well to see whether there's any objection from Lynx PCS users
to adding it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[Ioana Ciornei]

On Fri, Jun 02, 2023 at 11:30:36AM +0100, Russell King (Oracle) wrote:
> On Thu, Jun 01, 2023 at 05:17:04PM -0700, msmulski2@gmail.com wrote:
> > +/* USXGMII registers for Marvell switch 88e639x are undocumented and this function is based
> > + * on some educated guesses. It appears that there are no status bits related to
> > + * autonegotiation complete or flow control.
> > + */
> > +static int mv88e639x_serdes_pcs_get_state_usxgmii(struct mv88e6xxx_chip *chip,
> > +						  int port, int lane,
> > +						  struct phylink_link_state *state)
> > +{
> > +	u16 status, lp_status;
> > +	int err;
> > +
> > +	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > +				    MV88E6390_USXGMII_PHY_STATUS, &status);
> > +	if (err) {
> > +		dev_err(chip->dev, "can't read Serdes USXGMII PHY status: %d\n", err);
> > +		return err;
> > +	}
> > +	dev_dbg(chip->dev, "USXGMII PHY status: 0x%x\n", status);
> > +
> > +	state->link = !!(status & MDIO_USXGMII_LINK);
> > +
> > +	if (state->link) {
> > +		err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > +					    MV88E6390_USXGMII_LP_STATUS, &lp_status);
> 
> What's the difference between these two registers? Specifically, what
> I'm asking is why the USXGMII partner status seems to be split between
> two separate registers.
> 
> Note that I think phylink_decode_usxgmii_word() is probably missing a
> check for MDIO_USXGMII_LINK, based on how Cisco SGMII works (and
> USXGMII is pretty similar.)
> 
> MDIO_USXGMII_LINK indicates whether the attached PHY has link on the
> media side or not. It's entirely possible for the USXGMII link to be
> up (thus allowing us to receive the "LPA" from the PHY) but for the
> PHY to be reporting to us that it has no media side link.
> 
> So, I think phylink_decode_usxgmii_word() at least needs at the
> beginning this added:
> 
> 	if (!(lpa & MDIO_USXGMII_LINK)) {
> 		state->link = false;
> 		return;
> 	}
> 
> The only user of this has been the Lynx PCS, so I'll add Ioana to this
> email as well to see whether there's any objection from Lynx PCS users
> to adding it.
>

I just tested this snippet on a LX2160ARDB that has USXGMII on two of
its lanes which go to Aquantia AQR113C PHYs.

The lpa read is 0x5601 and, with the patch, the interface does not link
up. I am not sure what is happening, if it's this PHY in particular that
does not properly set MDIO_USXGMII_LINK.

Ioana

Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[Russell King (Oracle)]

On Fri, Jun 02, 2023 at 02:28:04PM +0300, Ioana Ciornei wrote:
> On Fri, Jun 02, 2023 at 11:30:36AM +0100, Russell King (Oracle) wrote:
> > On Thu, Jun 01, 2023 at 05:17:04PM -0700, msmulski2@gmail.com wrote:
> > > +/* USXGMII registers for Marvell switch 88e639x are undocumented and this function is based
> > > + * on some educated guesses. It appears that there are no status bits related to
> > > + * autonegotiation complete or flow control.
> > > + */
> > > +static int mv88e639x_serdes_pcs_get_state_usxgmii(struct mv88e6xxx_chip *chip,
> > > +						  int port, int lane,
> > > +						  struct phylink_link_state *state)
> > > +{
> > > +	u16 status, lp_status;
> > > +	int err;
> > > +
> > > +	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > > +				    MV88E6390_USXGMII_PHY_STATUS, &status);
> > > +	if (err) {
> > > +		dev_err(chip->dev, "can't read Serdes USXGMII PHY status: %d\n", err);
> > > +		return err;
> > > +	}
> > > +	dev_dbg(chip->dev, "USXGMII PHY status: 0x%x\n", status);
> > > +
> > > +	state->link = !!(status & MDIO_USXGMII_LINK);
> > > +
> > > +	if (state->link) {
> > > +		err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > > +					    MV88E6390_USXGMII_LP_STATUS, &lp_status);
> > 
> > What's the difference between these two registers? Specifically, what
> > I'm asking is why the USXGMII partner status seems to be split between
> > two separate registers.
> > 
> > Note that I think phylink_decode_usxgmii_word() is probably missing a
> > check for MDIO_USXGMII_LINK, based on how Cisco SGMII works (and
> > USXGMII is pretty similar.)
> > 
> > MDIO_USXGMII_LINK indicates whether the attached PHY has link on the
> > media side or not. It's entirely possible for the USXGMII link to be
> > up (thus allowing us to receive the "LPA" from the PHY) but for the
> > PHY to be reporting to us that it has no media side link.
> > 
> > So, I think phylink_decode_usxgmii_word() at least needs at the
> > beginning this added:
> > 
> > 	if (!(lpa & MDIO_USXGMII_LINK)) {
> > 		state->link = false;
> > 		return;
> > 	}
> > 
> > The only user of this has been the Lynx PCS, so I'll add Ioana to this
> > email as well to see whether there's any objection from Lynx PCS users
> > to adding it.
> >
> 
> I just tested this snippet on a LX2160ARDB that has USXGMII on two of
> its lanes which go to Aquantia AQR113C PHYs.
> 
> The lpa read is 0x5601 and, with the patch, the interface does not link
> up. I am not sure what is happening, if it's this PHY in particular that
> does not properly set MDIO_USXGMII_LINK.

Thanks for testing. I wonder if the USXGMII control word that the PHY
transmits can be read from one of its registers?

If the PHY is correctly setting the link bit, but it's not appearing
in the Lynx PCS registers as set, that would be weird, and suggest the
PCS is handling that itself. Does the Lynx link status bit change
according to the media link status, while the AN complete bit stays
set?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[Ioana Ciornei]

On Fri, Jun 02, 2023 at 02:11:14PM +0100, Russell King (Oracle) wrote:
> On Fri, Jun 02, 2023 at 02:28:04PM +0300, Ioana Ciornei wrote:
> > On Fri, Jun 02, 2023 at 11:30:36AM +0100, Russell King (Oracle) wrote:
> > > On Thu, Jun 01, 2023 at 05:17:04PM -0700, msmulski2@gmail.com wrote:
> > > > +/* USXGMII registers for Marvell switch 88e639x are undocumented and this function is based
> > > > + * on some educated guesses. It appears that there are no status bits related to
> > > > + * autonegotiation complete or flow control.
> > > > + */
> > > > +static int mv88e639x_serdes_pcs_get_state_usxgmii(struct mv88e6xxx_chip *chip,
> > > > +						  int port, int lane,
> > > > +						  struct phylink_link_state *state)
> > > > +{
> > > > +	u16 status, lp_status;
> > > > +	int err;
> > > > +
> > > > +	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > > > +				    MV88E6390_USXGMII_PHY_STATUS, &status);
> > > > +	if (err) {
> > > > +		dev_err(chip->dev, "can't read Serdes USXGMII PHY status: %d\n", err);
> > > > +		return err;
> > > > +	}
> > > > +	dev_dbg(chip->dev, "USXGMII PHY status: 0x%x\n", status);
> > > > +
> > > > +	state->link = !!(status & MDIO_USXGMII_LINK);
> > > > +
> > > > +	if (state->link) {
> > > > +		err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > > > +					    MV88E6390_USXGMII_LP_STATUS, &lp_status);
> > > 
> > > What's the difference between these two registers? Specifically, what
> > > I'm asking is why the USXGMII partner status seems to be split between
> > > two separate registers.
> > > 
> > > Note that I think phylink_decode_usxgmii_word() is probably missing a
> > > check for MDIO_USXGMII_LINK, based on how Cisco SGMII works (and
> > > USXGMII is pretty similar.)
> > > 
> > > MDIO_USXGMII_LINK indicates whether the attached PHY has link on the
> > > media side or not. It's entirely possible for the USXGMII link to be
> > > up (thus allowing us to receive the "LPA" from the PHY) but for the
> > > PHY to be reporting to us that it has no media side link.
> > > 
> > > So, I think phylink_decode_usxgmii_word() at least needs at the
> > > beginning this added:
> > > 
> > > 	if (!(lpa & MDIO_USXGMII_LINK)) {
> > > 		state->link = false;
> > > 		return;
> > > 	}
> > > 
> > > The only user of this has been the Lynx PCS, so I'll add Ioana to this
> > > email as well to see whether there's any objection from Lynx PCS users
> > > to adding it.
> > >
> > 
> > I just tested this snippet on a LX2160ARDB that has USXGMII on two of
> > its lanes which go to Aquantia AQR113C PHYs.
> > 
> > The lpa read is 0x5601 and, with the patch, the interface does not link
> > up. I am not sure what is happening, if it's this PHY in particular that
> > does not properly set MDIO_USXGMII_LINK.
> 
> Thanks for testing. I wonder if the USXGMII control word that the PHY
> transmits can be read from one of its registers?
> 

I had a look into the AQR gen4 datasheet and the only thing that I could
find is the "PCS USX0 Auto-Neg Control Register" which has the following
field:

	USX0 Auto-Negotiation Message code [7:0]
	Configure the message opcode for USXGMII Auto-Negotiation.

It sounded promising but it's not making any sense to me because it's
just 8 bits long.

> If the PHY is correctly setting the link bit, but it's not appearing
> in the Lynx PCS registers as set, that would be weird, and suggest the
> PCS is handling that itself. Does the Lynx link status bit change
> according to the media link status, while the AN complete bit stays
> set?
>

No, the Lynx link status bit is 1 even though the media side is no
longer connected, for example. Here I just removed the cable, the link
on the PHY is down but the PCS link is up.

[  133.011696] fsl_dpaa2_eth dpni.2 endpmac3: Link is Down
[  133.555343] phylink_decode_usxgmii_word 3331: lpa 0x5601
[  133.560672] mdio_bus 0x0000000008c0f000:00: mode=usxgmii/10Gbps/Full link=1 an_complete=1
[  134.579348] phylink_decode_usxgmii_word 3331: lpa 0x5601


Ioana

RE: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[Michal Smulski]

I spent some time today poking around pcs-lynx driver in hopes of using it to learn more about the Marvell switch.

1. lynx_pcs_get_state_usxgmii() is the function that the pcs-lynx driver is using to poll the status and to get LPA word. After I trigger auto-negotiation and it completes MII_LPA=0xd601. Therefore, the proposed change to phylink_decode_usxgmii_word() below would work fine with Marvell switch.
2. lynx_pcs_config_usxgmii() is where the driver sets its MII_ADVERTISE bits. However, I noted that Marvell switch achieves link status (10G) and obtains MII_LPA (reg=lp_status) from LX2162a BEFORE this function is executed (I traced this with printk() but I am fairly sure this is true given that I trigger DPMAC13 up manually). So, it seems that default settings on LX2162a allow for getting link (on Marvell switch). I was trying to force LX2162 to advertise speed lower then 10G to see how and when Marvell switch would respond but I was not able to figure this out. Please let me know if you are aware of an easy way to do disable default settings and force LX2162 to wait until lynx_pcs_config_usxgmii() is called to set link speed/duplex mode?

Thank you,
Michal 

-----Original Message-----
From: Ioana Ciornei <ioana.ciornei@nxp.com> 
Sent: Friday, June 2, 2023 7:32 AM
To: Russell King (Oracle) <linux@armlinux.org.uk>
Cc: msmulski2@gmail.com; andrew@lunn.ch; f.fainelli@gmail.com; olteanv@gmail.com; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; simon.horman@corigine.com; kabel@kernel.org; Michal Smulski <michal.smulski@ooma.com>
Subject: Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

CAUTION: This email is originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.


On Fri, Jun 02, 2023 at 02:11:14PM +0100, Russell King (Oracle) wrote:
> On Fri, Jun 02, 2023 at 02:28:04PM +0300, Ioana Ciornei wrote:
> > On Fri, Jun 02, 2023 at 11:30:36AM +0100, Russell King (Oracle) wrote:
> > > On Thu, Jun 01, 2023 at 05:17:04PM -0700, msmulski2@gmail.com wrote:
> > > > +/* USXGMII registers for Marvell switch 88e639x are 
> > > > +undocumented and this function is based
> > > > + * on some educated guesses. It appears that there are no 
> > > > +status bits related to
> > > > + * autonegotiation complete or flow control.
> > > > + */
> > > > +static int mv88e639x_serdes_pcs_get_state_usxgmii(struct mv88e6xxx_chip *chip,
> > > > +                                                 int port, int lane,
> > > > +                                                 struct 
> > > > +phylink_link_state *state) {
> > > > +       u16 status, lp_status;
> > > > +       int err;
> > > > +
> > > > +       err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > > > +                                   MV88E6390_USXGMII_PHY_STATUS, &status);
> > > > +       if (err) {
> > > > +               dev_err(chip->dev, "can't read Serdes USXGMII PHY status: %d\n", err);
> > > > +               return err;
> > > > +       }
> > > > +       dev_dbg(chip->dev, "USXGMII PHY status: 0x%x\n", 
> > > > + status);
> > > > +
> > > > +       state->link = !!(status & MDIO_USXGMII_LINK);
> > > > +
> > > > +       if (state->link) {
> > > > +               err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > > > +                                           
> > > > + MV88E6390_USXGMII_LP_STATUS, &lp_status);
> > >
> > > What's the difference between these two registers? Specifically, 
> > > what I'm asking is why the USXGMII partner status seems to be 
> > > split between two separate registers.
> > >
> > > Note that I think phylink_decode_usxgmii_word() is probably 
> > > missing a check for MDIO_USXGMII_LINK, based on how Cisco SGMII 
> > > works (and USXGMII is pretty similar.)
> > >
> > > MDIO_USXGMII_LINK indicates whether the attached PHY has link on 
> > > the media side or not. It's entirely possible for the USXGMII link 
> > > to be up (thus allowing us to receive the "LPA" from the PHY) but 
> > > for the PHY to be reporting to us that it has no media side link.
> > >
> > > So, I think phylink_decode_usxgmii_word() at least needs at the 
> > > beginning this added:
> > >
> > >   if (!(lpa & MDIO_USXGMII_LINK)) {
> > >           state->link = false;
> > >           return;
> > >   }
> > >
> > > The only user of this has been the Lynx PCS, so I'll add Ioana to 
> > > this email as well to see whether there's any objection from Lynx 
> > > PCS users to adding it.
> > >
> >
> > I just tested this snippet on a LX2160ARDB that has USXGMII on two 
> > of its lanes which go to Aquantia AQR113C PHYs.
> >
> > The lpa read is 0x5601 and, with the patch, the interface does not 
> > link up. I am not sure what is happening, if it's this PHY in 
> > particular that does not properly set MDIO_USXGMII_LINK.
>
> Thanks for testing. I wonder if the USXGMII control word that the PHY 
> transmits can be read from one of its registers?
>

I had a look into the AQR gen4 datasheet and the only thing that I could find is the "PCS USX0 Auto-Neg Control Register" which has the following
field:

        USX0 Auto-Negotiation Message code [7:0]
        Configure the message opcode for USXGMII Auto-Negotiation.

It sounded promising but it's not making any sense to me because it's just 8 bits long.

> If the PHY is correctly setting the link bit, but it's not appearing 
> in the Lynx PCS registers as set, that would be weird, and suggest the 
> PCS is handling that itself. Does the Lynx link status bit change 
> according to the media link status, while the AN complete bit stays 
> set?
>

No, the Lynx link status bit is 1 even though the media side is no longer connected, for example. Here I just removed the cable, the link on the PHY is down but the PCS link is up.

[  133.011696] fsl_dpaa2_eth dpni.2 endpmac3: Link is Down [  133.555343] phylink_decode_usxgmii_word 3331: lpa 0x5601 [  133.560672] mdio_bus 0x0000000008c0f000:00: mode=usxgmii/10Gbps/Full link=1 an_complete=1 [  134.579348] phylink_decode_usxgmii_word 3331: lpa 0x5601


Ioana

RE: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[Michal Smulski]

The two registers are not documented in Marvell spec for this switch and that is why my original patch did not have link status code.  I have followed suggestion by Andrew's and used other Marvell hardware that has USXGMII to see some of the same registers and same address would be accessible this switch (best effort attempt). 
These two registers appear to be read-only and likely are (1) what the switch is advertising on USXMII link and (2) what the switch received (link partner advertisement).
I am able to distinguish which is which because I can look at the other side's of USXGMII interfaces which has well defined USXGMII registers.

During the boot sequence these registers show the following:
1. On initial setup of the serdes link (inside the switch):
Status=0x00, lp_status=0xff
2. When irq link status triggers (mv88e6393x_serdes_irq_link_10g() ):
Status=0xd601, lp_status=0x9781
3. the driver then reports link
mv88e6085 0x0000000008b96000:02: Link is Up - 10Gbps/Full - flow control off

Please note that Link Partner' MDIO_USXGMII_LINK bit is always 1 which makes it useless for catching when the link is achieved. That is why I use switch's side link bit to note the link is set. However, I can certainly add this to the patch.

My board is based on LX2162a (a version of LX2160a). Specifically, USXMII interface is configured as follows:

LX2162a (DPMAC13 configured as USXGMII)  <---USXGMII --> 88E6191X (port10 configured as USXGMII).

In fact, I am using pcs-lynx driver for LX2162a side of the interface.

Michal


-----Original Message-----
From: Russell King <linux@armlinux.org.uk> 
Sent: Friday, June 2, 2023 3:31 AM
To: msmulski2@gmail.com; Ioana Ciornei <ioana.ciornei@nxp.com>
Cc: andrew@lunn.ch; f.fainelli@gmail.com; olteanv@gmail.com; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; simon.horman@corigine.com; kabel@kernel.org; Michal Smulski <michal.smulski@ooma.com>
Subject: Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

CAUTION: This email is originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.


On Thu, Jun 01, 2023 at 05:17:04PM -0700, msmulski2@gmail.com wrote:
> +/* USXGMII registers for Marvell switch 88e639x are undocumented and 
> +this function is based
> + * on some educated guesses. It appears that there are no status bits 
> +related to
> + * autonegotiation complete or flow control.
> + */
> +static int mv88e639x_serdes_pcs_get_state_usxgmii(struct mv88e6xxx_chip *chip,
> +                                               int port, int lane,
> +                                               struct 
> +phylink_link_state *state) {
> +     u16 status, lp_status;
> +     int err;
> +
> +     err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> +                                 MV88E6390_USXGMII_PHY_STATUS, &status);
> +     if (err) {
> +             dev_err(chip->dev, "can't read Serdes USXGMII PHY status: %d\n", err);
> +             return err;
> +     }
> +     dev_dbg(chip->dev, "USXGMII PHY status: 0x%x\n", status);
> +
> +     state->link = !!(status & MDIO_USXGMII_LINK);
> +
> +     if (state->link) {
> +             err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> +                                         MV88E6390_USXGMII_LP_STATUS, 
> + &lp_status);

What's the difference between these two registers? Specifically, what I'm asking is why the USXGMII partner status seems to be split between two separate registers.

Note that I think phylink_decode_usxgmii_word() is probably missing a check for MDIO_USXGMII_LINK, based on how Cisco SGMII works (and USXGMII is pretty similar.)

MDIO_USXGMII_LINK indicates whether the attached PHY has link on the media side or not. It's entirely possible for the USXGMII link to be up (thus allowing us to receive the "LPA" from the PHY) but for the PHY to be reporting to us that it has no media side link.

So, I think phylink_decode_usxgmii_word() at least needs at the beginning this added:

        if (!(lpa & MDIO_USXGMII_LINK)) {
                state->link = false;
                return;
        }

The only user of this has been the Lynx PCS, so I'll add Ioana to this email as well to see whether there's any objection from Lynx PCS users to adding it.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[Russell King (Oracle)]

On Thu, Jun 01, 2023 at 05:17:04PM -0700, msmulski2@gmail.com wrote:
> +static int mv88e639x_serdes_pcs_get_state_usxgmii(struct mv88e6xxx_chip *chip,
> +						  int port, int lane,
> +						  struct phylink_link_state *state)
> +{
> +	u16 status, lp_status;
> +	int err;
> +
> +	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> +				    MV88E6390_USXGMII_PHY_STATUS, &status);
> +	if (err) {
> +		dev_err(chip->dev, "can't read Serdes USXGMII PHY status: %d\n", err);
> +		return err;
> +	}
> +	dev_dbg(chip->dev, "USXGMII PHY status: 0x%x\n", status);
> +
> +	state->link = !!(status & MDIO_USXGMII_LINK);

Another thing which is missing is filling in state->an_complete. Does
the PHY have a status bit for this when operating in USXGMII mode?
If not, please set it to state->link.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[Michal Smulski]

I do not know if there is a an_complete bit as per my previous email. I think for now I will set state->an_complete = state->link.
Michal

-----Original Message-----
From: Russell King <linux@armlinux.org.uk> 
Sent: Friday, June 2, 2023 4:08 AM
To: msmulski2@gmail.com
Cc: andrew@lunn.ch; f.fainelli@gmail.com; olteanv@gmail.com; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; simon.horman@corigine.com; kabel@kernel.org; Michal Smulski <michal.smulski@ooma.com>
Subject: Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

CAUTION: This email is originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.


On Thu, Jun 01, 2023 at 05:17:04PM -0700, msmulski2@gmail.com wrote:
> +static int mv88e639x_serdes_pcs_get_state_usxgmii(struct mv88e6xxx_chip *chip,
> +                                               int port, int lane,
> +                                               struct 
> +phylink_link_state *state) {
> +     u16 status, lp_status;
> +     int err;
> +
> +     err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> +                                 MV88E6390_USXGMII_PHY_STATUS, &status);
> +     if (err) {
> +             dev_err(chip->dev, "can't read Serdes USXGMII PHY status: %d\n", err);
> +             return err;
> +     }
> +     dev_dbg(chip->dev, "USXGMII PHY status: 0x%x\n", status);
> +
> +     state->link = !!(status & MDIO_USXGMII_LINK);

Another thing which is missing is filling in state->an_complete. Does the PHY have a status bit for this when operating in USXGMII mode?
If not, please set it to state->link.

Thanks.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!