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

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

[msmulski2]

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

Changelist:
* do not enable USXGMII for 6361 chips
* track state->an_complete with state->link 

*** BLURB HERE ***

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 | 47 ++++++++++++++++++++++++++++--
 drivers/net/dsa/mv88e6xxx/serdes.h |  4 +++
 4 files changed, 53 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH net-next v7 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 | 47 ++++++++++++++++++++++++++++--
 drivers/net/dsa/mv88e6xxx/serdes.h |  4 +++
 4 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 2af0c1145d36..8b51756bd805 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -812,11 +812,10 @@ static void mv88e6393x_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
 			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); */
 		}
 	}
 
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..a28b368ed016 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,42 @@ 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);
+	state->an_complete = state->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 +1056,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 +1212,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 +1253,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 +1518,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 +1624,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 v7 0/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[Andrew Lunn]

On Sun, Jun 04, 2023 at 10:39:53PM -0700, msmulski2@gmail.com wrote:
> From: Michal Smulski <michal.smulski@ooma.com>
> 
> Changelist:
> * do not enable USXGMII for 6361 chips
> * track state->an_complete with state->link 
> 
> *** BLURB HERE ***

Hi Michal

Please remember to remove the *** BLURB HERE ***. What often happens
is that a branch is created for a patchset, and then the branch is
merged and the content of patch 0/X is used as the merge commit
message. So this can end up in the git history.

For a single patch, you are not required to have a patch 0/X.

	 Andrew

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

[Michal Smulski]

Andrew,

I will remove this line and resend v7. This was a mistake on my part. However, could you clarify on what is the best way to let reviewers know what changed between different version of the same patch? It seemed to me that changlist should not be part of the git commit message and hence I decided to add 'cover-letter' email for each new version of the patch so that it would not be part of the applied patch to net-next git repo (but it would also be easy to match changelist email with patch email to people reviewing latest patch)

Thank you,
Michal

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch> 
Sent: Monday, June 5, 2023 5:42 AM
To: msmulski2@gmail.com
Cc: 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; kabel@kernel.org; ioana.ciornei@nxp.com; Michal Smulski <michal.smulski@ooma.com>
Subject: Re: [PATCH net-next v7 0/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 Sun, Jun 04, 2023 at 10:39:53PM -0700, msmulski2@gmail.com wrote:
> From: Michal Smulski <michal.smulski@ooma.com>
>
> Changelist:
> * do not enable USXGMII for 6361 chips
> * track state->an_complete with state->link
>
> *** BLURB HERE ***

Hi Michal

Please remember to remove the *** BLURB HERE ***. What often happens is that a branch is created for a patchset, and then the branch is merged and the content of patch 0/X is used as the merge commit message. So this can end up in the git history.

For a single patch, you are not required to have a patch 0/X.

         Andrew

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

[Andrew Lunn]

On Mon, Jun 05, 2023 at 04:32:03PM +0000, Michal Smulski wrote:
> Andrew,
> 

> I will remove this line and resend v7. This was a mistake on my
> part. However, could you clarify on what is the best way to let
> reviewers know what changed between different version of the same
> patch? It seemed to me that changlist should not be part of the git
> commit message and hence I decided to add 'cover-letter' email for
> each new version of the patch so that it would not be part of the
> applied patch to net-next git repo (but it would also be easy to
> match changelist email with patch email to people reviewing latest
> patch)

Some people think the history is actually useful, it shows what has
been considered etc and the patch matured.

However, anything text after the --- marker in a patch will get
discarded by git am when the patch is merged. So you can place the
history there.

	Andrew

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

[Russell King (Oracle)]

On Sun, Jun 04, 2023 at 10:39:54PM -0700, msmulski2@gmail.com wrote:
> 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 | 47 ++++++++++++++++++++++++++++--
>  drivers/net/dsa/mv88e6xxx/serdes.h |  4 +++
>  4 files changed, 53 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 2af0c1145d36..8b51756bd805 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -812,11 +812,10 @@ static void mv88e6393x_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
>  			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); */
>  		}
>  	}
>  
> 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..a28b368ed016 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,42 @@ 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);
> +	state->an_complete = state->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);
> 

Please put a comment here that lp_status appears to include the "link"
bit as per USXGMII spec.

We don't know about pcs-lynx yet, which doesn't _seem_ to with the
AR113C, and we don't know why - if we made phylink_decode_usxgmii_word()
parse this bit as well, we need to keep track of which implementations
do provide that bit and which do not (and may be buggy.)

> +		phylink_decode_usxgmii_word(state, lp_status);
> +	}
>  	return 0;
>  }
>  
> @@ -1020,6 +1056,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 +1212,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 +1253,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 +1518,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 +1624,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
> 
> 

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